-
Notifications
You must be signed in to change notification settings - Fork 1
Added support for trusted profile as an input #63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for trusted profile as an input #63
Conversation
|
/run pipeline |
|
@sai-madhav-k you must be in the team github-collaborators in order to trigger the pipeline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally speaking the functionality created by this PR is not clear, is there an issue bound to this PR?
Could be possible to have an example for the secret that needs to be created?
Could be possible to have a documentation showing what we are creating?
The PR hasn't any description, and it is also marked as new major version
Last, why are we adding support for trusted profile only for the dockerjson chain of secret? if it could be bound to an apikey value, wouldn't make sense to add the support for standard secrets too?
modules/eso-external-secret/main.tf
Outdated
| # validation for dockerjsonconfig secrets chain -> if it is a chain the kube secret type must be dockerconfigjson and sm secret type iam_credentials | ||
| validate_condition_chain = local.is_dockerjsonconfig_chain == true && (var.es_kubernetes_secret_type != "dockerconfigjson" || var.sm_secret_type != "iam_credentials") # checkov:skip=CKV_SECRET_6: does not require high entropy string as is static value | ||
| validate_msg_chain = "If the externalsecret is expected to generate a dockerjsonconfig secrets chain the only supported value for es_kubernetes_secret_type is dockerconfigjson and for sm_secret_type is iam_credentials" | ||
| # validation for dockerjsonconfig secrets chain -> if it is a chain the kube secret type must be dockerconfigjson and sm secret types iam_credentials, trusted_profile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of comma an 'or' would be more clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been resolved
modules/eso-external-secret/main.tf
Outdated
| validate_msg_chain = "If the externalsecret is expected to generate a dockerjsonconfig secrets chain the only supported value for es_kubernetes_secret_type is dockerconfigjson and for sm_secret_type is iam_credentials" | ||
| # validation for dockerjsonconfig secrets chain -> if it is a chain the kube secret type must be dockerconfigjson and sm secret types iam_credentials, trusted_profile | ||
| validate_condition_chain = local.is_dockerjsonconfig_chain == true && (var.es_kubernetes_secret_type != "dockerconfigjson" || (var.sm_secret_type != "iam_credentials" && var.sm_secret_type != "trusted_profile")) # checkov:skip=CKV_SECRET_6: does not require high entropy string as is static value | ||
| validate_msg_chain = "If the externalsecret is expected to generate a dockerjsonconfig secrets chain the only supported value for es_kubernetes_secret_type is dockerconfigjson and for sm_secret_type is iam_credentials and trusted_profile" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean iam_credentials or trusted_profile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modules/eso-external-secret/main.tf
Outdated
| (element.trusted_profile != null && element.trusted_profile != "") ? | ||
| { | ||
| "username" : element.trusted_profile, "password" : "{{ .secretid_${index} }}" | ||
| }: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if at line #49 you specify that the username is apikey for trustedprofile, why do you put a different username here?
In addition, what's the expected username?
Last: has a trusted profile an apikey?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the following line can be avoided as I am giving the username in the variable data_payload_chain_map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, so you can remove it
modules/eso-external-secret/main.tf
Outdated
| resource "helm_release" "kubernetes_secret_chain_list_tp" { | ||
| count = local.is_dockerjsonconfig_chain == true && var.sm_secret_type=="trusted_profile" ? 1 : 0 | ||
| name = local.helm_secret_name | ||
| namespace = local.es_helm_rls_namespace | ||
| chart = "${path.module}/../../chart/${local.helm_raw_chart_name}" | ||
| version = local.helm_raw_chart_version | ||
| timeout = 600 | ||
| values = [ | ||
| <<-EOF | ||
| resources: | ||
| - apiVersion: external-secrets.io/v1beta1 | ||
| kind: ExternalSecret | ||
| metadata: | ||
| name: "${var.es_kubernetes_secret_name}" | ||
| namespace: "${var.es_kubernetes_namespace}" | ||
| spec: | ||
| refreshInterval: ${var.es_refresh_interval} | ||
| secretStoreRef: | ||
| name: "${var.eso_store_name}" | ||
| kind: "${local.secret_store_ref_kind}" | ||
| target: | ||
| name: "${var.es_kubernetes_secret_name}" | ||
| template: | ||
| engineVersion: v2 | ||
| type: "${local.es_kubernetes_secret_type}" | ||
| metadata: | ||
| annotations: | ||
| ${local.reloader_annotation} | ||
| data: | ||
| ${local.data_chain} | ||
| data: | ||
| %{for index, element in var.es_container_registry_secrets_chain~} | ||
| - secretKey: secretid_${index} | ||
| remoteRef: | ||
| key: "${element.sm_secret_id}" | ||
| %{endfor~} | ||
| EOF | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for duplicating the code of kubernetes_secret_chain_list? the only difference that I see is the value of sm_secret_type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its better to have a separate block for trusted-profile's dockerconfigjson, since the format of giving the secret id is different. The below is reference for secret if the selected secret type is trusted_profile
remoteRef:
key: "${element.sm_secret_id}"
whereas for the normal dockerconfigjson it is :
remoteRef:
key: "${var.sm_secret_type}/${element.sm_secret_id}"
I thought its better to give it like this, since there might be little chance of breaking things if anything goes wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would mean to duplicate the code for a little change, you could differentiate the string to put here with a condition and keep the block not duplicated
For all Dreadnought cabins we need enable the capability to pull images from ibm-dreadnought-prod-images using Trusted profile onboarding into AppGalley account. This is how the secret need to look for accessing the images from
We have the following the trusted profile in the appgalley account: Issue: https://github.ibm.com/dreadnought/kanban/issues/3026 |
Ok thank you now it is more clear to me.
by implementing the condition this would avoid to have this condition here
Last, you are implementing only for registry secrets chain, if we support it in the chain, we need to add its support in the standard registry secret configuration, if it is a valid use-case |
I think we are using this variable in another variable called |
|
@sai-madhav-k could be possible to add the same option for the single secret version?
|
|
Modified the block for both trusted profile and iam_credentials. As we are linking trusted profile with service id, so the service id apikey will be in the secrets manager under iam credentials, I have added it as |
@vbontempi We can do it in subsequent release. For now can we release it for trusted profile as container_registry_chain. In the subsequent weeks I will create a different PR for this. |
|
@vbontempi can you review this PR today , if there are any shortcomings it can be rectified within a day. Thank you ! |
|
The following are the helm values after deployment of secrets
|
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
For all Dreadnought cabins we need enable the capability to pull images from ibm-dreadnought-prod-images using Trusted profile onboarding into AppGalley account.
This is how the secret need to look for accessing the images from
ibm-dreadnought-prod-imagesWe have the following the trusted profile in the appgalley account:
dn-dev-d-appgalley-icr-dn-base-images-tp. This trusted profile will be having the access to pull dreadnought provided images. A trust relationship will be established between service id and the trusted profile. Once the trust relationship is established between service id and trusted profile, this service id apikey needs to be shipped into cluster and has to be deployed as dockerconfigjson, so that cabin teams can refer to the trusted profile and service id apikey for pulling images from appgalley account.Issue: https://github.ibm.com/dreadnought/kanban/issues/3026
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers